Summary

Changes

Please provide a summary of what's being changed

This PR improves the decorator logic of the AppSyncEventsResolver by fixing a fundamental issue that prevented it from being useful.

Prior to this fix the decorators would consistently cause the decorated methods to become unbound and lose the reference to this, preventing customers to access other class methods or properties from within the decorated methods.

This was happening because unlike other decorator implementations in the codebase, the one in this area of the codebase works in two steps:

1. at init time the decorator saves a reference of the decorated method for later usage
2. at runtime the app.resolve method can call a decorated method depending on routing decisions

By the time the reference is called, the initial scope is lost causing the issue. At the same time, the this scope is not yet available at init time - this is because decorators are evaluated before class init - so it's not possible to save a reference or bind this along with the the decorated method.

Because of this, we need to get around this decorator design aspect.

In first iteration of the fix I was introspecting the class methods, find their definition, and when we encounter the .resolve keyword (which represents the method), saved a reference to this for later use. This reference was then used and passed to the decorated method to apply the correct this value (see example below).

/**
   * Binds the resolve method scope to the target object.
   *
   * We patch any method that might call `resolve()` to ensure that
   * the class instance is captured correctly when the method is resolved. We need
   * to do this because when a method is decorated, it loses its context and
   * the `this` keyword inside the method no longer refers to the class instance of the decorated method.
   *
   * We need to apply this two-step process because the decorator is applied to the method
   * before the class instance is created, so we cannot capture the instance directly.
   *
   * @param target - The target object whose methods will be patched to capture the instance scope
   */
  #bindResolveMethodScope(target: object) {
    const routerInstance = this;

    // Patch any method that might call resolve() to capture instance
    if (!target.constructor.prototype._powertoolsPatched) {
      target.constructor.prototype._powertoolsPatched = true;

      // Get all method names from the prototype
      const proto = target.constructor.prototype;
      const methodNames = Object.getOwnPropertyNames(proto);

      for (const methodName of methodNames) {
        if (methodName === 'constructor') continue;

        const methodDescriptor = Object.getOwnPropertyDescriptor(
          proto,
          methodName
        );
        if (
          methodDescriptor?.value &&
          typeof methodDescriptor.value === 'function'
        ) {
          const originalMethodRef = methodDescriptor.value;
          const methodSource = originalMethodRef.toString();

          // Check if this method calls .resolve() on our router instance
          if (
            methodSource.includes('.resolve(') ||
            methodSource.includes('.resolve ')
          ) {
            const patchedMethod = function (this: unknown, ...args: unknown[]) {
              // Capture instance when any method that calls resolve is called
              if (this && typeof this === 'object') {
                routerInstanceMap.set(routerInstance, this);
              }
              return originalMethodRef.apply(this, args);
            };

            Object.defineProperty(proto, methodName, {
              value: patchedMethod,
              writable: true,
              configurable: true,
              enumerable: true,
            });
          }
        }
      }
    }
  }

After further consideration, as well as advice from @svozza, I instead went in a different direction and moved to something more explicit.

When using the decorator pattern, customers now must pass a reference tothis when calling app.resolve(), for example:

const app = new AppSyncEventsResolver({ logger: console });

class Lambda {
  public scope = 'scoped';

  @app.onSubscribe('/foo')
  public async handleFoo(payload: AppSyncEventsSubscribeEvent) {
    console.debug(`${this.scope} ${payload.info.channel.path}`);
  }

  public async handler(event: unknown, context: Context) {
    return this.stuff(event, context);
  }

  async stuff(event: unknown, context: Context) {
    return app.resolve(event, context, { scope: this });
  }
}
const lambda = new Lambda();
const handler = lambda.handler.bind(lambda);

This ensures that the scope is propagated and properly applied when calling the decorated method at runtime.

While requiring an additional step from customers, I prefer this solution as it's more explicit and clear rather than relying on some implicit behavior that could result in unknown results when bundling/minifying the code.

Note that this applies only to the decorator pattern, when using the functional approach the parameter is ignored.

Please add the issue number below, if no issue is present the PR might get blocked and not be reviewed

Issue number: fixes #3973

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Disclaimer: We value your time and bandwidth. As such, any pull requests created on non-triaged issues might not be successful.